fix(policy): update policy_map by calling add_policy#377
Closed
Victor-Bayim wants to merge 3 commits intocasbin:masterfrom
Closed
fix(policy): update policy_map by calling add_policy#377Victor-Bayim wants to merge 3 commits intocasbin:masterfrom
Victor-Bayim wants to merge 3 commits intocasbin:masterfrom
Conversation
|
|
Victor-Bayim
changed the title
Victor-Bayim
changed the title
Victor-Bayim
force-pushed
the
fix/add_policies_policy_map
branch
from
0fdb343 to
c3c3c78
Compare
hsluoyz
requested changes
Mar 20, 2025
Victor-Bayim
force-pushed
the
fix/add_policies_policy_map
branch
from
c3c3c78 to
46ba668
Compare
Victor-Bayim
force-pushed
the
fix/add_policies_policy_map
branch
from
068de06 to
11079f3
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where add_policies did not update the policy_map correctly by invoking add_policy for each new rule.
- Rules are now added via add_policy in add_policies to ensure proper updates to both the policy list and the policy_map.
- The update_policy and update_policies methods have been revised to update the policy_map consistently, and the remove_* functions now rebuild policy_map after modifications.
Comments suppressed due to low confidence (1)
casbin/model/policy.py:175
- The removal of the p_priority check in update_policy changes the behavior by allowing rule priority to be updated without validation. Verify that this change is intentional and consider reintroducing a safeguard if maintaining the existing priority is required.
ast.policy[rule_index] = new_rule
Comment on lines
+216
to
+219
| new_map = {} | ||
| for idx, r in enumerate(assertion.policy): | ||
| new_map[DEFAULT_SEP.join(r)] = idx | ||
| assertion.policy_map = new_map |
There was a problem hiding this comment.
[nitpick] The logic to rebuild policy_map is duplicated in several methods (remove_policy, remove_filtered_policy_returns_effects, remove_filtered_policy). Consider refactoring this into a helper function to improve maintainability.
Suggested change
| new_map = {} | |
| for idx, r in enumerate(assertion.policy): | |
| new_map[DEFAULT_SEP.join(r)] = idx | |
| assertion.policy_map = new_map | |
| assertion.policy_map = self._rebuild_policy_map(assertion.policy) |
Member
|
Replaced by: #390 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This Pull Request fixes the issue where add_policies does not update the internal policy_map when adding new policies (see #356). Previously, add_policies simply appended new rules to the policy list without invoking add_policy, causing priority and other related information to be missing from policy_map. The main changes include:
Invoke add_policy for Each Rule
In add_policies, each new rule is added via the add_policy method to ensure that both the policy list and the policy_map are updated.
This approach is aligned with the Go Casbin implementation for consistency.
Duplicate Check
If any rule already exists, the function returns False to prevent adding duplicates.
Testing
All existing test cases have been run locally, and they pass successfully.
Additional tests for priority-based rules can be added in the future to further confirm that policy_map is correctly updated.
By applying this fix, add_policies now properly maintains the internal data structures, preventing access control logic errors due to an out-of-sync policy_map. Feedback and reviews are welcome!